-
-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make #[derive(PostgresType)]
impl its own FromDatum
#1381
Make #[derive(PostgresType)]
impl its own FromDatum
#1381
Conversation
The only downstream user, as far as I can tell, that this might negatively affect, is @pksunkara's pgx-ulid. If you genuinely don't want to impl/derive Serialize and Deserialize for your extension's type, lmk and I'll figure out an alternative. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please let me know if I am misunderstanding something.
It looks like we are making this change to improve the UX of the errors and there's nothing that will change the functionality of IntoDatum
and FromDatum
.
I have a suggestion to allow skipping specifying Serialize
and Deserialize
for users who implement their own IntoDatum
and FromDatum
. Have a proc macro called PostgresDatum
or something that implements these and enforces the Serialize
and Deserialize
bounds. I think this would solve #965 too.
@pksunkara Everything is going to be completely reworked, including the FromDatum and IntoDatum traits, and I don't see why the Serialize and Deserialize bounds are involved at all in your suggestion. ( Apologies if you saw the earlier draft, I misread something. ) |
It would also not solve #965, because it does not actually do what is needed in order to actually make Postgres use those functions for binary send/receive, which is to export a function that can be called by Postgres. |
There is a bunch of important background information to take in before we go with that idea, @pksunkara: The definition of the FromDatum trait itself is effectively unsound. All implementations are not merely of Thus, before we make a full release of 0.12.0, there will need to be a new trait or set of traits defining type translations to/from Datums, and they will not be able to use the usual Currently I am making a bunch of small changes like this one to "clear the field" and make migration easier. Importantly, I am trying to do so while not disturbing the majority of code which depends on the current interfaces, so that most users will be able to simply write With all that in mind, I honestly would rather not add an entirely new I am more unsure what you get out of the |
I think I have a custom implementation of |
if that's true, then what actually happens if you remove the PostgresType derive??? |
Never tried it since I didn't know what it contained. FWIW, I have |
@pksunkara In any case, while I expressed some reticence, I am happy to work out a hack so you can write something other than This would go on the large pile of TODOs blocking 0.12 though, as we have several at this point, and I would prefer it if we could open an issue and figure out what actually needs to happen. And yes, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, at least for now. Long term I think we'll want flexibility here.
I thnk there are a lot of cases where users will want to control this. I remember a while looking into why a pgrx implementation of roaring bitmaps was slower than a C one (specifically https://github.com/ChenHuajun/pg_roaringbitmap, I believe) and one of the big reasons was our encoding/decoding overhead. |
@thomcc Right. I want to leave the possibility of control in but I think we need a new mechanism for enabling it, although it might be because we want to put PostgresType as-is out to pasture and bring it back in a more modular form. |
There is now a ...Please yell at me if that actually makes it into a final release without getting at least a rename. |
The deprecation in question is the hidden function, |
Mainly, this removes a source of persistent and confounding type errors because of the generic blanket impl of FromDatum for all T that fulfill so-and-so bounds. These may mislead one, if one is writing code generic over FromDatum, to imagine that one needs a Serialize and Deserialize impl or bound for a given case, even when those are not required. By moving these requirements onto the type that derives, this moves any confusion to the specific cases it actually applies to.
This has a regrettable effect that now PostgresType requires a Serialize and Deserialize impl in order to work, unless one uses the hacky
#[bikeshed_postgres_type_manually_impl_from_into_datum]
attribute, which I intend to rename or otherwise fix up before pgrx reaches its 0.12.0 release.